Re: [PATCH V4 01/24] x86/ioreq: Prepare IOREQ feature for making it common

[Oleksandr <olekstysh@xxxxxxxxx>]

On 15.01.21 18:41, Jan Beulich wrote:

Hi Jan

> On 12.01.2021 22:52, Oleksandr Tyshchenko wrote:
> > @@ -1080,6 +1104,27 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain 
> > *d, ioservid_t id,
> >       return rc;
> >   }
> >   
> > +/* Called with ioreq_server lock held */
> > +int arch_ioreq_server_map_mem_type(struct domain *d,
> > +                                   struct hvm_ioreq_server *s,
> > +                                   uint32_t flags)
> > +{
> > +    return p2m_set_ioreq_server(d, flags, s);
> > +}
> > +
> > +void arch_ioreq_server_map_mem_type_completed(struct domain *d,
> > +                                              struct hvm_ioreq_server *s,
> > +                                              uint32_t flags)
> > +{
> > +    if ( flags == 0 )
> > +    {
> > +        const struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +
> > +        if ( read_atomic(&p2m->ioreq.entry_count) )
> > +            p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> If I was the maintainer of this code, I'd ask that such single
> use variables, unless needed to sensibly deal with line length
> restrictions, be removed.
ok, will remove. With that we could omit the braces and have a combined 
condition on a single line.

> > --- a/xen/include/asm-x86/hvm/ioreq.h
> > +++ b/xen/include/asm-x86/hvm/ioreq.h
> > @@ -19,6 +19,9 @@
> >   #ifndef __ASM_X86_HVM_IOREQ_H__
> >   #define __ASM_X86_HVM_IOREQ_H__
> >   
> > +#define HANDLE_BUFIOREQ(s) \
> > +    ((s)->bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF)
> > +
> >   bool hvm_io_pending(struct vcpu *v);
> >   bool handle_hvm_io_completion(struct vcpu *v);
> >   bool is_ioreq_server_page(struct domain *d, const struct page_info *page);
> > @@ -55,6 +58,25 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered);
> >   
> >   void hvm_ioreq_init(struct domain *d);
> >   
> > +bool arch_vcpu_ioreq_completion(enum hvm_io_completion io_completion);
> > +int arch_ioreq_server_map_pages(struct hvm_ioreq_server *s);
> > +void arch_ioreq_server_unmap_pages(struct hvm_ioreq_server *s);
> > +void arch_ioreq_server_enable(struct hvm_ioreq_server *s);
> > +void arch_ioreq_server_disable(struct hvm_ioreq_server *s);
> > +void arch_ioreq_server_destroy(struct hvm_ioreq_server *s);
> > +int arch_ioreq_server_map_mem_type(struct domain *d,
> > +                                   struct hvm_ioreq_server *s,
> > +                                   uint32_t flags);
> > +void arch_ioreq_server_map_mem_type_completed(struct domain *d,
> > +                                              struct hvm_ioreq_server *s,
> > +                                              uint32_t flags);
> > +bool arch_ioreq_server_destroy_all(struct domain *d);
> > +bool arch_ioreq_server_get_type_addr(const struct domain *d,
> > +                                     const ioreq_t *p,
> > +                                     uint8_t *type,
> > +                                     uint64_t *addr);
> > +void arch_ioreq_domain_init(struct domain *d);
> As indicated before, I don't think these declarations should
> live here. Even if a later patch moves them I wouldn't see
> why they couldn't be put in their final resting place right
> away.
Well, will introduce common ioreq.h right away.


> Also where possible without violating line length restrictions
> please still try to put multiple parameters on a single line,
> as is done higher up in this file.
Got it.


> Jan
--
Regards,

Oleksandr Tyshchenko