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

[Max Reitz]

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.)

>> +    /* 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.)

> 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?

Max

signature.asc
Description: OpenPGP digital signature