Re: gnumach: [PATCH] - irq as a mach device

[Damien Zammit]

On 9/7/20 9:45 am, Samuel Thibault wrote:
> Thanks! It looks good, just a couple last things:
> 
> - you don't need to introduce and call mach_cli(), splhigh already
>   disables interrupts, see the implementation in i386/i386/spl.S

ACK

> - Please keep intr.h's __INTR_H__ guard for consistency.

ACK

> - When moving code around, you have to copy over the licence statements,
>   so here the gpl2+ statement of irq.c
>   Ideally we'd dive back to check whether the mask_irq and unmask_irq
>   functions could be available under a BSD licence, but gpl2+ is
>   compatible with this BSD licence so it's fine to append the gpl2+
>   notice.

ACK

> - For new files, also add licensing terms. Since gnumach is mostly BSD
>   we can continue with BSD, see the terms I added to intr.[ch].
ACK

>> + *     We can specify physical memory range limits and alignment.
> 
> pmax is ambiguous, it could be understood as the last possible byte
> address, please document that it is the address of the byte just after
> the limit, e.g. 0x100000000 for limiting addresses to 32bits.

ACK
 
> For paddr, we really want to use phys_addr_t, otherwise we're stuck in
> the 4GiB area.

ACK

> I don't think you need to distinguish VM_PAGE_MAX_SEGS == 4 from == 3.
> Something like this should be fine:
> #ifdef VM_PAGE_SEL_DMA32
>               selector = VM_PAGE_SEL_DMA32;
>       if (pmax > VM_PAGE_DMA32_LIMIT)
> #endif

ACK

>> +    size = vm_page_round(size + palign);
> 
>>      alloc_size = (1 << (order + PAGE_SHIFT));
>>      npages = vm_page_atop(alloc_size);
> 
> I don't think you need to add palign, that could allocate twice as much
> as what you need when the size is equel to palign for instance. The
> alloc_size will be a power of two already, so you just need to bump
> order to vm_page_order(palign) if needed.

I don't think that is correct either.  The physical alignment of the memory
is unrelated to the size requested.  I thought I could hack it but it doesn't 
work
so I've left physical alignment as a TODO as well as pmin != 0, but included it
in the RPC itself so it can be committed even if it doesn't do anything yet.

Please see attached 2 new patches.

Thanks,
Damien

0001-irq-device.patch
Description: Text Data

0002-mach-vm_allocate_contiguous.patch
Description: Text Data