Re: [PATCH] ide:do nothing for identify cmd if no any device attached

[John Snow]

On 9/3/20 6:40 AM, Max Reitz wrote:
> On 02.09.20 20:02, John Snow wrote:
> > (CC Max for block backend model confusion, see below)
> >
> > On 8/16/20 11:38 PM, zhaoxin\RockCuioc wrote:
> > > This patch is for avoiding win7 IDE driver polling 0x1f7 when
> > > no any device attached. During Win7 VM boot procedure, if use virtio for
> > > disk and there is no any device be attached on hda & hdb, the win7 IDE
> > > driver
> > > would poll 0x1f7 for a while. This action may be stop windows LOGO
> > > atomic for
> > > a while too on a poor performance CPU.
> >
> > A few questions:
> >
> > (1) How slow is the probing?
> >
> > (2) If there are no devices attached, why don't you remove the IDE
> > controller so that Windows doesn't have to probe it?
> >
> > > Signed-off-by: zhaoxin\RockCuioc <RockCui-oc@zhaoxin.com>
> > > ---
> > >    hw/ide/core.c | 5 +++--
> > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > > index d997a78e47..26d86f4b40 100644
> > > --- a/hw/ide/core.c
> > > +++ b/hw/ide/core.c
> > > @@ -2073,8 +2073,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
> > >        s = idebus_active_if(bus);
> > >        trace_ide_exec_cmd(bus, s, val);
> > >    -    /* ignore commands to non existent slave */
> > > -    if (s != bus->ifs && !s->blk) {
>
> (Was the first one basically meant to be “s != &bus->ifs[0]”, i.e. to
> check that this doesn’t go to the ma^W primary?  Not too obvious.)

Yeah, I think it was meant to say:

if (s == bus->ifs[1] && !s->blk)

(But I don't know why it was important to guard device1 specifically. 
Knowledge lost to the sands of time.)

By the way, the correct terms are "device0" and "device1" and have been 
since at least ATA2. I believe ATA1 used the terms "disk0" and "disk1". 
"Primary" and "Secondary" are used to refer to the controllers, not the 
devices.

- Primary
  - device0
  - device1
- Secondary
  - device0
  - device1

Thanks for coming to my TED talk!

> > > +    /* ignore commands if no any device exist or non existent slave */
> > > +    if ((!bus->ifs[0].blk && !bus->ifs[1].blk) ||
> > > +        (s != bus->ifs && !s->blk)) {
>
> (Maybe this could be improved here)
>
> > >            return;
> > >        }
> > >    
> >
> > I think it's the case that Empty CD-ROM drives will have an anonymous
> > block backend representing the empty drive,
>
> (As far as I remember,) yes.
>
> (ide_dev_initfn() ensures all CD drives have one, even if it’s empty.)

(Thanks)

> > so I suppose this is maybe
> > fine?
> >
> > I suppose the idea is that with no drives on the bus that it's fine to
> > ignore the register writes, as there are no devices to record those writes.
> >
> > (But then, why did we ever only check device1? ...)
> >
> > Maybe before the block-backend split we used to have to check to see if
> > we had attached media or not, but I think nowadays we should always have
> > a blk pointer if we have a device model intended to be operating at this
> > address.
>
> The check in ide_dev_initfn() looks that way to me.
>
> > So I guess it can be simplified ...?
> >
> > if (!s->blk) {
> >      return;
> > }
>
> Probably.  Although there’s a difference, of course, namely if you have
> only a secondary device and try to access the primary, this simplified
> version will be a no-op, whereas the more complicated version in this
> patch would still go on.  I don’t know how real hardware would handle
> that case.  Is it even possible to have just a secondary with no primary?

I think so. From what I understand, two drives on a single channel both 
receive all of the same register update commands, including the "SELECT" 
register, which has a bit devoted to which drive we have selected.

When we write to the COMMAND register, only the selective drive should 
actually respond to it.

so what I expect happens on real machines:

- You select device0
- You write to a bunch of registers
- You issue a command
- Nobody responds.

--js