On Tue, Mar 28, 2023 at 03:03:17PM +0200, Jan Beulich wrote:
> On 28.03.2023 14:52, Marek Marczykowski-Górecki wrote:
> > On Tue, Mar 28, 2023 at 02:34:23PM +0200, Jan Beulich wrote:
> >> On 28.03.2023 14:05, Marek Marczykowski-Górecki wrote:
> >>> On Tue, Mar 28, 2023 at 01:28:44PM +0200, Roger Pau Monné wrote:
> >>>> On Sat, Mar 25, 2023 at 03:49:23AM +0100, Marek Marczykowski-Górecki
> >>>> wrote:
> >>>>> +static bool cf_check msixtbl_page_accept(
> >>>>> + const struct hvm_io_handler *handler, const ioreq_t *r)
> >>>>> +{
> >>>>> + ASSERT(r->type == IOREQ_TYPE_COPY);
> >>>>> +
> >>>>> + return msixtbl_page_handler_get_hwaddr(
> >>>>> + current->domain, r->addr, r->dir == IOREQ_WRITE);
> >>>>
> >>>> I think you want to accept it also if it's a write to the PBA, and
> >>>> just drop it. You should always pass write=false and then drop it in
> >>>> msixtbl_page_write() if it falls in the PBA region (but still return
> >>>> X86EMUL_OKAY).
> >>>
> >>> I don't want to interfere with msixtbl_mmio_page_ops, this handler is
> >>> only about accesses not hitting actual MSI-X structures.
> >>
> >> In his functionally similar vPCI change I did ask Roger to handle the
> >> "extra" space right from the same handlers. Maybe that's going to be
> >> best here, too.
> >
> > I have considered this option, but msixtbl_range() is already quite
> > complex, adding yet another case there won't make it easier to follow.
>
> Do you care about the case of msixtbl_addr_to_desc() returning NULL at
> all for the purpose you have? IIUC I care specifically about this case. > Like in Roger's patch I'd assume > msixtbl_find_entry() needs extending what ranges it accepts; if need > be another parameter may be added to cover cases where the extended > coverage isn't wanted. > > > I mean, technically I can probably merge those two handlers together, > > but I don't think it will result in nicer code. Especially since the > > general direction is to abandon split of MSI-X table access handling > > between Xen and QEMU and go with just QEMU doing it, hopefully at some > > point not needing msixtbl_mmio_ops anymore (but still needing the one > > for adjacent accesses). > > Hmm, at this point I'm not convinced of this plan. Instead I was hoping > that once vPCI properly supports PVH DomU-s, we may also be able to make > use of it for HVM, delegating less to qemu rather than more. In that case, this code won't be needed anymore, which will also make this handler unnecessary. Anyway, I tried to merge this handling into existing handlers and the resulting patch is slightly bigger, so it doesn't seem to avoid any duplication. The only benefit I can think of is avoiding iterating msixtbl_list twice (for respective accept callbacks) on each access. Is it worth a bit more complicated handlers? -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab
signature.asc
Description: PGP signature
