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.

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) {
+    /* 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)) {
         return;
     }
 
-- 
2.17.1

Patchew URL: https://patchew.org/QEMU/20200817033803.14014-1-RockCui-oc@zhaoxin.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-unit: tests/test-char
Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181:
attempt to add duplicate property 'serial-id' to object (type 'container')
ERROR test-char - too few tests run (expected 38, got 9)
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-qtest-x86_64: tests/qtest/hd-geo-test
  TEST    check-qtest-x86_64: tests/qtest/boot-order-test
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=8c36fb1727574c069b2f14432c2bddf7', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-0cmc1v90/src/docker-src.2020-08-17-00.03.16.18596:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=8c36fb1727574c069b2f14432c2bddf7
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-0cmc1v90/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    12m14.785s
user    0m8.902s


The full log is available at
http://patchew.org/logs/20200817033803.14014-1-RockCui-oc@zhaoxin.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

(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) {
> +    /* 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)) {
>           return;
>       }
>   

I think it's the case that Empty CD-ROM drives will have an anonymous 
block backend representing the empty drive, 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.

So I guess it can be simplified ...?

if (!s->blk) {
     return;
}

--js

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

Hi John & Max，


  1.  Follow my Log，there are 10000 read 0x1x7 ops. On my Intel I5 platform, if down the frequency to 1.2G, you can see a obviously lag during WINDOWS LOGO animation playing.
  2.  We must supply a CD-ROM to our VDI users.
  3.  In ide_ioport_read() :

---------------------------------------------------------------------------------------------------
case 7:
       if ((!bus->ifs[0].blk && !bus->ifs[1].blk) || (s != bus->ifs && !s->blk)) {
            ret = 0;
       } else {
            ret = s->status;
       }
----------------------------------------------------------------------------------------------------
            so I follow this.

Rock



________________________________
发件人: Max Reitz <mreitz@redhat.com>
发送时间: 2020年9月3日 18:40
收件人: John Snow; RockCui-oc; qemu-devel@nongnu.org
抄送: Cobe Chen(BJ-RD); Peter Maydell
主题: Re: [PATCH] ide:do nothing for identify cmd if no any device attached

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

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

Hi all，


Any suggestions for this patch？

Rock

________________________________
发件人: John Snow <jsnow@redhat.com>
发送时间: 2020年10月3日 3:37
收件人: Max Reitz; RockCui-oc; qemu-devel@nongnu.org
抄送: Peter Maydell; Cobe Chen(BJ-RD)
主题: Re: [PATCH] ide:do nothing for identify cmd if no any device attached

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