Re: [PATCH v2 01/35] xen: introduce resource.h

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Tuesday, December 10th, 2024 at 5:13 AM, Jan Beulich <jbeulich@xxxxxxxx> 
wrote:

>
>
> On 06.12.2024 05:41, Denis Mukhin via B4 Relay wrote:
>
> > --- /dev/null
> > +++ b/xen/include/xen/resource.h
> > @@ -0,0 +1,40 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
>
> GPL-2.0-only

Fixed.

>
> > +/*
> > + * System resource description.
> > + *
> > + * Reference:
> > + * include/linux/ioport.h
>
>
> I'm unsure of the usefulness of such a reference.

These definitions are taken from Linux'es
  include/linux/ioport.h

I think reference to the original code may help developers to borrow more
missing declarations in the future.

But sure, removed.

>
> > + */
> > +#if !defined(XEN__RESOURCE_H)
>
>
> Nit: #ifdef / #ifndef please whenever possible (as long as not inconsistent
> with adjacent code).

Sure.

>
> > +#define XEN__RESOURCE_H
> > +
> > +#define IORESOURCE_BITS 0x000000FFU /* Bus-specific bits /
> > +
> > +#define IORESOURCE_TYPE_BITS 0x00001F00U / Resource type /
> > +#define IORESOURCE_IO 0x00000100U / PCI/ISA I/O ports /
> > +#define IORESOURCE_MEM 0x00000200U
> > +#define IORESOURCE_REG 0x00000300U / Register offsets /
> > +#define IORESOURCE_IRQ 0x00000400U
> > +#define IORESOURCE_DMA 0x00000800U
> > +#define IORESOURCE_BUS 0x00001000U
> > +
> > +#define IORESOURCE_PREFETCH 0x00002000U / No side effects */
> > +#define IORESOURCE_READONLY 0x00004000U
> > +#define IORESOURCE_CACHEABLE 0x00008000U
> > +#define IORESOURCE_RANGELENGTH 0x00010000U
> > +#define IORESOURCE_SHADOWABLE 0x00020000U
> > +
> > +#define IORESOURCE_UNKNOWN (~0U)
> > +
> > +struct resource {
> > + paddr_t addr;
> > + paddr_t size;
> > + unsigned int type;
> > +};
> > +
> > +#define resource_size(res) (res)->size;
>
>
> The semicolon surely was wrong before and is wrong now. Plus Misra
> demands that such macro expansions be parenthesized, I think.

Fixed.

>
> > +#define foreach_resource(res) \
> > + for (; res && res->type != IORESOURCE_UNKNOWN; res++)
>
>
> This one isn't being moved, but is being added. It's not used here,
> which makes it difficult to judge its correctness. Perhaps better to
> introduce this when its first needed, and then right away with the
> required parentheses around uses of the macro parameter.

I moved that hunk into the place where it is first used (the emulator
patch).

>
> > +#endif /* #if !defined(XEN__RESOURCE_H) */
>
>
> Just the guard identifier in the comment please.

Done.

>
> Jan