Re: [Xen-devel] [PATCH QEMU-XEN] xen/pt: Start with emulated PCI_COMMAND set to zero.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.06.15 at 18:19, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Thu, 11 Jun 2015, Jan Beulich wrote:
>> >>> On 10.06.15 at 22:53, <konrad@xxxxxxxxxx> wrote:
>> > --- a/hw/xen/xen_pt.c
>> > +++ b/hw/xen/xen_pt.c
>> > @@ -785,7 +785,9 @@ out:
>> >          xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
>> >                                pci_get_word(d->config + PCI_COMMAND) | 
>> > cmd);
>> >      }
>> > -
>> > +    /* Until the guest enables the device use d->config values which will
>> > +     * inhibit pci_bar_address & pci_update_mappings from triggering 
>> > updates.*/
>> > +    pci_set_word(d->config + PCI_COMMAND, 0);
>> >      memory_listener_register(&s->memory_listener, &address_space_memory);
>> >      memory_listener_register(&s->io_listener, &address_space_io);
>> >      XEN_PT_LOG(d,
>> 
>> Well, I can see this as something to be tried out as an experiment,
>> but it looks like you mean this to be a proper submission for
>> inclusion upstream? Or maybe not, considering that qemu-devel
>> wasn't even Cc-ed? In any case - what we need here is a general
>> solution to at least the initialization part of the problem, i.e. all
>> fields we emulate some or all bits for need to have d->config[]
>> updated accordingly (i.e. you need to merge d->config[] and
>> XenPTReg's data field based on the respective XenPTRegInfo's
>> emu_mask, but perhaps simply copying the data field to
>> d->config[] would have the same effect; if it doesn't, we have
>> yet another problem). For the command register this for example
>> means that it is in no way guaranteed that it would end up being
>> zero; its emu_mask however guarantees that the memory and I/O
>> decode bits would start out as zero (which is what you're after).
> 
> I am not sure I can ask Konrad to come up with a larger general solution
> when his intent was just to fix this issue.  This one liner is OK for
> that.

But apart from leaving the same issue around for all other fields
the change above is just a hack anyway, going from one incorrect
value (the host one) to another incorrect one (constant zero).
What I'd consider acceptable as a partial solution would be if at
least proper merging was done for this particular field. Yet I
suppose once doing that, it's not going to be that much more
work to do at least proper init-value merging for all fields. Doing
the wider change of eliminating/replacing the data field would
indeed seem to go too far for an immediate fix of the issue here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel