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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Jun 16, 2015 at 08:33:32AM +0100, Jan Beulich wrote:
> >>> 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.

While digging in this I realized that some shortcuts and assumptions
had been taken (I think I am restating what you two have already
realized).

1) The dev.config is (by Xen code) used as the cache of the
   host configuration devices (which is OK at init right now).

   However to sync up the ->data with dev.config (and apply emu_mask)
   means that it cannot be used as such (the semantics of that have changed). 

2). The dev.config is (by the generic code) used as a view of what
   the guest should see (cache of guest values).


To make this work, the plan would be:

 1). Remove all the dev.config accesses to check host values:
        @@ -728,7 +729,7 @@ static int xen_pt_initfn(PCIDevice *d)
             }
         
             /* Bind interrupt */
        -    if (!s->dev.config[PCI_INTERRUPT_PIN]) {
        +    if (xen_host_pci_get_byte(PCI_INTERRUPT_PIN)) {
                 XEN_PT_LOG(d, "no pin interrupt\n");
                 goto out;
             }
     And replace them with calls to get the actual host value.

 2). Replace all the pci_get_[byte,word,long] (which are wrappers
     around dev.config) in the init routines (see get_capability_version and
     xen_pt_linkctrl_reg_init) with calls to xen_host_pci_[byte,word,long].

     In short - replace the calls to get the actual host values.

That would untangle a lot of this and make it a bit saner (I hope).

And after that I can:

 3). Rip out ->data and use pci_set_[byte,word,long] or pci_get_[byte,word,long]
     to get one unified view of what the guest is suppose to have.

 4). Tackle bugs that will creep up because of this. I am not sure what
     they are, but surely will hit some. I expect that some of these
     patches will add debug/more logging to help me tackle this.

 5). Reinstate an host cache (if needed) for configuration access to lessen
     the amount of reads we do. 'host_cache_config' perhaps?

Anything I missed?

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