On Wed, Dec 11, 2024 at 09:36:00AM +0000, Chen, Jiqian wrote:
> On 2024/12/11 16:16, Roger Pau Monné wrote:
> > On Wed, Dec 11, 2024 at 06:37:30AM +0000, Chen, Jiqian wrote:
> >> On 2024/12/10 19:25, Roger Pau Monné wrote:
> >>> On Tue, Dec 10, 2024 at 10:54:43AM +0100, Jan Beulich wrote:
> >>>> On 10.12.2024 08:57, Chen, Jiqian wrote:
> >>>>> On 2024/12/10 15:17, Jan Beulich wrote:
> >>>>>> On 10.12.2024 08:07, Chen, Jiqian wrote:
> >>>>>>> On 2024/12/9 21:59, Jan Beulich wrote:
> >>>>>>>> On 02.12.2024 07:09, Jiqian Chen wrote:
> >>>>>>>>> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
> >>>>>>>>> + unsigned int reg,
> >>>>>>>>> + uint32_t val,
> >>>>>>>>> + void *data)
> >>>>>>>>> +{
> >>>>>>>>> + uint64_t size;
> >>>>>>>>> + unsigned int index;
> >>>>>>>>> + struct vpci_bar *bars = data;
> >>>>>>>>> +
> >>>>>>>>> + if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) &
> >>>>>>>>> PCI_COMMAND_MEMORY )
> >>>>>>>>> + return;
> >>>>>>>>
> >>>>>>>> I don't think something like this can go uncommented. I don't think
> >>>>>>>> the
> >>>>>>>> spec mandates to drop writes in this situation?
> >>>>>>> Spec says: Software must clear the Memory Space Enable bit in the
> >>>>>>> Command register before writing the BAR Size field.
> >>>>>>> This check is suggested by Roger and it really helps to prevent
> >>>>>>> erroneous writes in this case,
> >>>>>>> such as the result of debugging with Roger in the previous version.
> >>>>>>> I will add the spec's sentences as comments here in next version.
> >>>>>>
> >>>>>> What you quote from the spec may not be enough as a comment here.
> >>>>>> There's
> >>>>>> no direct implication that the write would simply be dropped on the
> >>>>>> floor
> >>>>>> if the bit is still set. So I think you want to go a little beyond just
> >>>>>> quoting from the spec.
> >>>>> How about quoting Roger's previous words: " The memory decoding must be
> >>>>> disabled before writing the BAR size field.
> >>>>> Otherwise changing the BAR size will lead to the active p2m mappings
> >>>>> getting out of sync w.r.t. the new BAR size." ?
> >>>>
> >>>> That'll be better, but imo still not enough to explain the outright
> >>>> ignoring
> >>>> of the write.
> >>>
> >>> I think we might want to do something along the lines of:
> >>>
> >>> uint64_t size = PCI_REBAR_CTRL_SIZE(val);
> >>> struct vpci_bar *bar = data;
> >>>
> >>> if ( bar->enabled )
> >>> {
> >>> if ( size == bar->size )
> >>> return;
> >>>
> >>> /*
> >>> * Refuse to resize a BAR while memory decoding is enabled, as
> >>> * otherwise the size of the mapped region in the p2m would become
> >>> * stale with the newly set BAR size, and the position of the BAR
> >>> * would be reset to undefined. Note the PCIe specification also
> >>> * forbids resizing a BAR with memory decoding enabled.
> >>> */
> >>> gprintk(XENLOG_ERR,
> >>> "%pp: refuse to resize BAR with memory decoding enabled\n",
> >>> &pci->sbdf);
> >>> return;
> >>> }
> >> Thank you very much!
> >>
> >>>
> >>> Note this requires that the data parameter points to the BAR that
> >>> matches the ReBAR control register, this needs adjusting in
> >>> init_rebar().
> >> I think I can keep current implementation of init_rebar() and use
> >> bars[index] to get the corresponding BAR.
> >
> > IMO it would be best if you can pass the corresponding bar struct into
> > the handler directly, as that will avoid having to do a PCI read just
> > to get the BAR index from PCI_REBAR_CTRL. It should also avoid the
> > need for the index and BAR type checks in rebar_ctrl_write().
> OK, if so, then I need to move the logic of getting index from PCI_REBAR_CTRL
> register and checking the BAR type into init_rebar(), right?
Yes, I think that would be better, as then the check is done only once
at init rather than on every access.
Thanks, Roger.
