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