Re: [Xen-devel] [PATCH v6 08/11] vpci/bars: add handlers to map the BARs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.10.17 at 14:02, <roger.pau@xxxxxxxxxx> wrote:
> On Thu, Oct 05, 2017 at 11:55:39AM +0000, Jan Beulich wrote:
>> >>> On 05.10.17 at 13:09, <roger.pau@xxxxxxxxxx> wrote:
>> > On Thu, Oct 05, 2017 at 10:01:46AM +0000, Jan Beulich wrote:
>> >> >>> On 05.10.17 at 11:20, <roger.pau@xxxxxxxxxx> wrote:
>> >> > On Wed, Oct 04, 2017 at 08:33:33AM +0000, Jan Beulich wrote:
>> >> >> >>> On 19.09.17 at 17:29, <roger.pau@xxxxxxxxxx> wrote:
>> >> >> > +            bool rom_enabled;
>> >> >> 
>> >> >> Especially with the error handling issue in mind that I've mentioned
>> >> >> earlier, I wonder whether this field shouldn't be dropped, along the
>> >> >> lines of you also no longer caching the memory decode enable bit in the
>> >> >> command register.
>> >> > 
>> >> > Removing rom_enabled would imply doing a register read in
>> >> > vpci_modify_bars in order to know whether the ROM BAR is enabled or
>> >> > not, which is not trivial because depending on the header type the
>> >> > position of the ROM BAR is different.
>> >> 
>> >> As said - I wouldn't mind the field if it was always in sync with the
>> >> hardware one. And it was for a reason that I mentioned the
>> >> memory decode bit, which you no longer cache. I think both
>> >> should be treated the same.
>> > 
>> > I think I'm missing something, rom_enabled matches exactly the state
>> > of the ROM enable bit. There's no way rom_enabled will get updated
>> > without the BAR ROM also being updated in vpci_rom_write.
>> 
>> Oh, I'm sorry for not being precise here: I think the hardware
>> bit should only be set once the mapping is complete. That's
>> not how the code currently behaves, so yes, right now the
>> cached bit apparently properly reflects the actual one. With
>> the possibly deferred mapping, that wouldn't be the case.
> 
> I could add some tail code to vpci_process_pending that sets the
> memory decoding or ROM BAR enable bit together with the rom_enable and
> enabled fields in the header struct. Would you agree to this?

If that's cleanly doable, sure. I had assumed you didn't do it
because you couldn't reasonably update state at that later point.

Jan


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